-
Notifications
You must be signed in to change notification settings - Fork 766
Update django form mutation to more granularly handle input/output fields #934
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update django form mutation to more granularly handle input/output fields #934
Conversation
Thanks @jtratner ! I get the use case for this but I'm just wondering if this is the best API for it because it seems quite verbose. Could we simply it by just having 2 options: Related: for DjangoObjectType's we are planning on removing the |
@jkimbo - totally agree that it’s wordy 😂😅 I was trying to follow the existing convention. Honestly if I were designing this class from the start I prob would have users explicitly specify output fields for the mutation. I’m not clear on a use case where you’d want to post the fields from If you wanted to do that string, I’d suggest defining a constant
|
I think that api makes more sense @jtratner so let's move towards that implementation. We can first start by preferring Don't have a strong opinion on defining a constant verses just writing out |
What about getting rid of the automatic output fields in a backwards compatible way:
This would make the API much simpler |
I think I like the idea of defaulting to no output fields which means you have to be very explicit about what outputs you want. Unfortunately I don't have many usecases for DjangoFormMutation so I'm not that familiar with what people use it form. What kind of things are you using it form @jtratner ? |
Our app (for good or ill) has a number of forms that are pretty-close-but-not-quite ModelForms. (So they essentially construct objects, just the input is different than underlying DB representation). This class is handy cuz it lets us reuse validation logic from our existing HTML front end (old school without JS) That said, looking at Django Rest Framework, I wonder if (Apologies for the array of ideas - just hoping to get to a meaningful API) |
(See #933 for a simplifies version of our use case) |
Set up forms Update docs Nearly working tests
a2da4ab
to
4246dab
Compare
|
||
result = schema.execute( | ||
""" mutation MyMutation { | ||
myMutation(input: { text: "VALID_INPUT" }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkimbo - I am super confused why this test errors with text not a valid keyword argument
when the assertion statements above are true. Any thoughts?
> assert result.errors is None
E assert [GraphQLLocatedError("'text' is an invalid keyword argument for MyMutation")] is None
E + where [GraphQLLocatedError("'text' is an invalid keyword argument for MyMutation")] = <graphql.execution.base.ExecutionResult object at 0x103d25af8>.errors```
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
@jkimbo - circling back to this after a while :) Has your API break completed now? (I think that was a blocker in the past). If so, is there a set way of naming that you'd like me to use? If not, I can try to get back in the zone for this PR and finish it up. |
Example of how I'd like to change DjangoFormMutation as per #933. I think this should be backwards compatible with the existing one. Check out docstring on
DjangoFormMutation
for an overview of what the changes (ought to!) do :)I also tried to add some docs about class behavior.
This still needs a test case and probably to be merged into the public graphene django docs but I figured this'd be good basis for discussion :)
Ultimately this is motivated by me having desire to be able to have a graphql mutation that just returns
clientMutationId
and a single object even tho I'm not using aDjangoModelFormMutation
.